Improve selection and merged cell announcements in LibreOffice Calc 7.3 and above #12849

LeonarddeR · 2021-09-15T10:31:38Z

Link to issue number:

Summary of the issue:

In LibreOffice calc, selection is not announced and for merged cells, only the first cell is announced.

Description of how this pull request fixes the issue:

In LibreOffice 7.3 development branch, support for IAccessibleTable2 and IAccessibleTableCell were added. This allows us to implement something that works as expected.
Daily builds that support this new functionality can be downloaded here

Testing strategy:

Merged several cells and observed that the announcement was appropriate.
Selected several cells and observed that the announcement was appropriate.

Known issues with pull request:

LibreOffice doesn't fire selectionRemove event properly. This is a known issue that's being investigated.

Change log entries:

Changes

NVDA will report selection and merged cells in LibreOffice Calc 7.3 and above, to be released in early 2022. (LibreOffice Calc: report merged cell coordinates #9310, LibreOffice Calc: report selected cell coordinates #6897)

Code Review Checklist:

Pull Request description is up to date.
Unit tests.
System (end to end) tests.
Manual testing.
API is compatible with existing addons.
User Documentation.
Change log entry.
Context sensitive help for GUI changes.
UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers

AppVeyorBot · 2021-09-15T10:48:52Z

Build (for testing PR): https://ci.appveyor.com/api/buildjobs/ymql2jamwflonw4a/artifacts/output/nvda_snapshot_pr12849-23758,1692477c.exe
PASS: Translation comments check.
FAIL: Lint check. See test results for more information.
PASS: System tests.
PASS: Unit tests.

See test results for failed build of commit 1692477c49

LeonarddeR · 2021-09-16T09:30:34Z

Thanks to @michaelweghorn who's doing all the great stuff at the Libre side.

source/appModules/soffice.py

s. discussion in nvaccess#9310

LeonarddeR · 2021-09-23T16:32:58Z

I proposed a general way to announce selection in #6959. If people agree, I want to generalize the selection approach in with a follow up.

source/NVDAObjects/IAccessible/__init__.py

seanbudd · 2021-09-27T00:47:05Z

source/appModules/soffice.py


-def gridCoordStringToNumbers(coordString):


Is the removal of these functions and classes going to affect add-ons?

That's very unlikely I think. Add-ons should avoid using appModules directly.

@feerrenrut @michaelDCurran what do you think?

@feerrenrut - requested you review pending this discussion, and my concerns including this in 2021.3

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

CyrilleB79 · 2021-09-27T08:35:58Z

@LeonarddeR you wrote:

That's very unlikely I think. Add-ons should avoid using appModules directly.

I think that this assumption is not correct.

In my Outlook Extended add-on, I import the native Outlook appModule to extend its capability.

The import statements are as follow:

# Import all from original outlook appModule to keep it available if needed.
from nvdaBuiltin.appModules.outlook import *
# Import here explecitely used variables from original outlook appModule for clarity.
from nvdaBuiltin.appModules.outlook import UIAGridRow, AddressBookEntry, AppModule

LeonarddeR · 2021-09-27T10:29:17Z

Indeed, the only reason why you would like to import an appModule directly is by extending it. With my statement, I meant that it is unlikely that an add-on would import a top level function from an appModule for another reason than extending the appModule. Having said that, I think that, while we should not break NVDA API compatibility other than in Addon API version changing releases, appModules shouldn't be treated as part of the NVDA API. NVDA makes this pretty clear by design, as many objects that are app specific yet reside in the core NVDAObjects package.

seanbudd · 2021-09-27T10:44:08Z

If this PR doesn't preserve API compatibility it will have to wait until 2022.1

LeonarddeR · 2021-09-27T11:37:30Z

I guess that depends on whether the cleanup I did in the soffice appModule for stuff that hasn't been used for years is considered API breaking?

seanbudd · 2021-10-13T21:58:24Z

This PR seems otherwise ready to go, other than API breaking concerns. Will aim to merge this in early once we branch for 2022.1

seanbudd

Thanks for the wait @LeonarddeR

Link to issue number: Fixes nvaccess#13232 Summary of the issue: Since commit 32bf54e ("Improve selection and merged cell announcements in LibreOffice Calc 7.3 and above (nvaccess#12849)"), information about selected cells in LibreOffice Calc is queried using the IAccessibleTable2 interface which is supported from LibreOffice 7.3 on. The call to the 'selectedCells' method on that interface requests a list of currently selected accessibles. Since Calc can have more than 1 billion cells, generating the accessible objects for currently selected cells can take a long time when many cells are selected. Description of how this pull request fixes the issue: Before calling the 'selectedCells' method, the number of currently selectd cells is retrieved first and the previous mechanism of announcing exact selected cells is only used if at most 2000 cells are selected. Otherwise, only the number of selected cells is announced. For the special case where all cells are selected (i.e. the number of selected cells is the same as the number of the table's children), "all cells" are reported as selected.

Link to issue number: Fixes nvaccess#13232 Summary of the issue: Since commit 32bf54e ("Improve selection and merged cell announcements in LibreOffice Calc 7.3 and above (nvaccess#12849)"), information about selected cells in LibreOffice Calc is queried using the 'IAccessibleTable2' interface which is supported from LibreOffice 7.3 on. The call to the 'selectedCells' method on that interface requests a list of a11y objects for all currently selected cells, of which only the first and the last one are actually needed for the announcement of selected cells. Since Calc spreadsheets have more than a billion cells, this is inefficient when many cells are selected and resulted in Calc becoming unresponsive. Description of how this pull request fixes the issue: Instead of using the 'selectedCells' method from the 'IAccessibleTable2' interface, the first and last selected cell are now retrieved using the 'accSelection' on the 'IAccessible' object of the table, which avoids that a11y objects for all other selected cells have to be generated as well.

Fixes #13232 Summary of the issue: Since PR #12849, information about selected cells in LibreOffice Calc is queried using the 'IAccessibleTable2' interface which is supported from LibreOffice 7.3 on. The call to the 'selectedCells' method on that interface requests a list of a11y objects for all currently selected cells, of which only the first and the last one are actually needed for the announcement of selected cells. Since Calc spreadsheets have more than a billion cells, this is inefficient when many cells are selected and resulted in Calc becoming unresponsive. Description of how this pull request fixes the issue: Instead of using the 'selectedCells' method from the 'IAccessibleTable2' interface, the first and last selected cell are now retrieved using the 'accSelection' on the 'IAccessible' object of the table, which avoids that a11y objects for all other selected cells have to be generated as well. Testing strategy: use LibreOffice Calc 7.3 or above select all cells in LO Calc and check that NVDA announces coordinate + content of the first cell (A1) and the last cell (AMJ1048576) in the spreadsheet test a few other selections in the spreadsheet (use shift + arrow keys to increase/decrease selection)

Improve selection and merged cell announcement in LibreOffice Calc

b9f7ee1

LeonarddeR added 3 commits September 16, 2021 11:22

Fix typo

f534f2d

Merge remote-tracking branch 'origin/master' into soffice

cedd3f0

Lint fix

e135386

lukaszgo1 reviewed Sep 16, 2021

View reviewed changes

source/appModules/soffice.py Outdated Show resolved Hide resolved

Import IA2 from IAccessibleHandler

efad4d4

michaelweghorn mentioned this pull request Sep 17, 2021

LibreOffice Calc: report merged cell coordinates #9310

Closed

michaelweghorn and others added 2 commits September 18, 2021 08:34

DEMO: reset IEnumVARIANT object before using it to retrieve child count

2db3707

s. discussion in nvaccess#9310

Clarification

5fa7c01

Merge remote-tracking branch 'origin/master' into soffice

8da36f6

LeonarddeR marked this pull request as ready for review September 24, 2021 05:59

LeonarddeR requested a review from a team as a code owner September 24, 2021 05:59

LeonarddeR requested a review from seanbudd September 24, 2021 05:59

seanbudd self-assigned this Sep 27, 2021

seanbudd reviewed Sep 27, 2021

View reviewed changes

Apply suggestions from code review

78c4402

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

seanbudd requested a review from feerrenrut October 5, 2021 06:51

seanbudd added this to the 2022.1 milestone Oct 13, 2021

seanbudd added 2 commits October 20, 2021 10:54

Merge remote-tracking branch 'origin/master' into soffice

bfe8453

update changes

d116976

seanbudd approved these changes Oct 19, 2021

View reviewed changes

seanbudd merged commit 32bf54e into nvaccess:master Oct 20, 2021

michaelweghorn mentioned this pull request Jan 12, 2022

Selecting all cells "freezes" LibreOffice Calc (when using LO >= 7.3, NVDA >= 2022.1) #13232

Closed

michaelweghorn mentioned this pull request Jan 12, 2022

soffice: Make selected cell announcement more efficient #13233

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve selection and merged cell announcements in LibreOffice Calc 7.3 and above #12849

Improve selection and merged cell announcements in LibreOffice Calc 7.3 and above #12849

LeonarddeR commented Sep 15, 2021 •

edited

AppVeyorBot commented Sep 15, 2021

LeonarddeR commented Sep 16, 2021

LeonarddeR commented Sep 23, 2021 •

edited

seanbudd Sep 27, 2021

LeonarddeR Sep 27, 2021

seanbudd Sep 27, 2021

seanbudd Oct 5, 2021 •

edited

CyrilleB79 commented Sep 27, 2021

LeonarddeR commented Sep 27, 2021

seanbudd commented Sep 27, 2021

LeonarddeR commented Sep 27, 2021

seanbudd commented Oct 13, 2021

seanbudd left a comment


		def gridCoordStringToNumbers(coordString):

Improve selection and merged cell announcements in LibreOffice Calc 7.3 and above #12849

Improve selection and merged cell announcements in LibreOffice Calc 7.3 and above #12849

Conversation

LeonarddeR commented Sep 15, 2021 • edited

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing strategy:

Known issues with pull request:

Change log entries:

Code Review Checklist:

AppVeyorBot commented Sep 15, 2021

LeonarddeR commented Sep 16, 2021

LeonarddeR commented Sep 23, 2021 • edited

seanbudd Sep 27, 2021

Choose a reason for hiding this comment

LeonarddeR Sep 27, 2021

Choose a reason for hiding this comment

seanbudd Sep 27, 2021

Choose a reason for hiding this comment

seanbudd Oct 5, 2021 • edited

Choose a reason for hiding this comment

CyrilleB79 commented Sep 27, 2021

LeonarddeR commented Sep 27, 2021

seanbudd commented Sep 27, 2021

LeonarddeR commented Sep 27, 2021

seanbudd commented Oct 13, 2021

seanbudd left a comment

Choose a reason for hiding this comment

LeonarddeR commented Sep 15, 2021 •

edited

LeonarddeR commented Sep 23, 2021 •

edited

seanbudd Oct 5, 2021 •

edited